SONARJAVA-6269 S1451 should correctly handle empty headerFormat#5580
SONARJAVA-6269 S1451 should correctly handle empty headerFormat#5580NoemieBenard merged 13 commits intomasterfrom
Conversation
SummaryRefactors
Includes test coverage for empty headerFormat in both regex and non-regex modes, plus the default (unconfigured) header behavior. What reviewers should knowWhere to start: Review the refactored Key behavior change: When Implementation details:
Tests to focus on:
|
There was a problem hiding this comment.
ClassDefaultHeader.java?
There was a problem hiding this comment.
Do you really need this file and RegexNoBlankLine.java? Why not reuse ClassBlankLine.java?
![sonar-review-alpha[bot]](https://avatars.githubusercontent.com/in/3025063?s=60&v=4){kind=small}
| check = new FileHeaderCheck(); | ||
| check.headerFormat = ""; | ||
| CheckVerifier.newVerifier() | ||
| .onFile(mainCodeSourcesPath("checks/FileHeaderCheck/ClassBlankLine.java")) | ||
| .withCheck(check) | ||
| .verifyNoIssues(); |
There was a problem hiding this comment.
ClassBlankLine.java starts with a blank line. Under the old code, "".split(…) produced [""], and matches([""], lines) compared the first line to "" — a blank-first-line file already passed. This test case would have been green before the fix too, so it doesn't verify anything new.
The real non-regex regression test is ClassNoBlankLine.java (first line is package …, which the old code incorrectly flagged). Consider replacing ClassBlankLine.java with a file that would have been mistakenly flagged under the old behavior — or just remove this case, since ClassNoBlankLine.java already covers the fix.
- Mark as noise
| CheckVerifier.newVerifier() | ||
| .onFile(mainCodeSourcesPath("checks/FileHeaderCheck/ClassBlankLine.java")) | ||
| .withCheck(check) | ||
| .verifyNoIssues(); |
There was a problem hiding this comment.
ClassBlankLine.java starts with a blank line (line 1 is ""). Under the old code, "".split("(?:\r)?\n|\r") returned [""], and matches([""], fileLines) compared the first line "" to the expected "" — a match, so no issue was reported. This test was already green before the fix and doesn't verify anything new for the non-regex path.
The real regression test for the non-regex fix is ClassNoBlankLine.java immediately below (first line is "package...", which the old code incorrectly flagged). Either remove the ClassBlankLine.java case here, or replace it with a comment explaining that it's not a regression test but a boundary-condition sanity check.
- Mark as noise
…ssue after the empty header fix
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| "CustomRepository:CustomMainCheck", | ||
| "CustomRepository:CustomJspCheck", | ||
| "CustomRepository:CustomTestCheck", | ||
| // not in SonarWay (FileHeaderCheck) |
There was a problem hiding this comment.
Does this test still need a fix? I could not reproduce it locally. If it does, could we find a different rule, not in SonarWay, which fails? Maybe S1166.
We can discuss it offline.
|
There was a problem hiding this comment.
LGTM! ✅
The new commit is a clean follow-up: S1451 no longer fires on the test files after the empty-header fix, so it's replaced with S1166 (CatchUsesExceptionWithContextCheck) in the sensor test to maintain coverage of the "not in SonarWay" assertion. No issues with this change. The two previously flagged concerns about ClassBlankLine.java test coverage in FileHeaderCheckTest remain unresolved.
2 participants
Refactor FileHeaderCheck to handle empty headers correctly.